Deeper configuration parsing and validation #515

paulo-ferraz-oliveira · 2025-07-15T21:27:37Z

Description

This pull request is all about:

configuration validation (check On config. validation #508)
preparation for some of the upcoming ergonomics/UX changes we have identified in Issues

I have read and understood the contributing guidelines

First of all, I want to acknowledge this is a big change and some (much?) stuff can change before it's considered for merging.

Two things stand out, to me:

I rebased on the latest 6 commits to the main branch and had 0 conflicts, which shows it's somewhat self-contained (even if BIG)
I want to update rebar3_lint and elvis in tandem with this, before it's considered for merging, because this a. touches on the API the aforementioned use, b. hopefully simplifies and gives utilities for a more unified experience in the Elvis ecosystem.

Required changes

I have updated rebar3_lint: Adapt to new elvis_core configuration validation project-fifo/rebar3_lint#72
I have updated elvis: Adapt to new elvis_core configuration validation elvis#596
I have written tests that touch on structure and type and as many exceptions as I can remember
I have self-reviewed everything I pushed
I have decided on showing results without throwing (kind of the current behaviour which prints sometimes and throws sometimes), and adapted tests to that

Closes #438.
Closes #439.

Further considerations

As always:

I'm opening this as draft but Ok to get comments on it
I haven't written tests yet, but have imagined how I'll do it (I basically adapted, up to now, the code for the tests to pass, trying to change them slightly, but not fundamentally)
I'll self-review my choices to allow for further discussion points, and these might also lead to further changes I'll eventually push

elbrujohalcon

A few comments…

README.md

src/elvis_config.erl

src/elvis_ruleset.erl

paulo-ferraz-oliveira · 2025-07-16T18:52:20Z

@elbrujohalcon, I'm doing "I have decided on showing results without throwing (kind of the current behaviour which prints sometimes and throws sometimes), and adapted tests to that".

My decision is to not throw for invalid_config, but return {fail, _}.

This:

eases consumption (the API is more predicable with a tuple than with a throw)
eases tests (no more unwarranted try-catchs)
lays the foundation for upcoming warnings_as_errors (for the time being, though, if there's issues detected with the configuration, elvis_core will signal it and analysis stops)

Edit: the remaining, explicit, throws are enoent and invalid_file for which we could apply a similar behaviour, in the future, if we wanted.

RULES.md

config/elvis-test-custom-ruleset.config

config/elvis-umbrella.config

rebar.config

src/elvis_core.erl

src/elvis_ruleset.erl

test/elvis_SUITE.erl

paulo-ferraz-oliveira · 2025-07-16T23:03:46Z

I've self-reviewed and (based on @bormilan's previous contributions) I enjoyed working with maybe. It makes for a simpler implementation and read, and approaches the Elixir-like zen I search when validating configurations.

src/elvis_core.erl

src/elvis_task.erl

(since we have no other state to work with, at the moment)

... and consumers

paulo-ferraz-oliveira · 2025-07-21T21:38:15Z

Made some minor adjustment (latest commits) from adapting to rebar3_lint, but as I've identified it I'm only missing tests on elvis_core, at this moment.

elbrujohalcon

I did a quick review today (after a few days travelling around Europe 😎 )… I just left a nit-picky comment. I'll do a more in-depth review later.

test/elvis_test_utils.erl

paulo-ferraz-oliveira · 2025-07-30T14:48:39Z

I love the all new ✅ with Windows in the mix.

I can't simply validate rules as being non-empty because it is, by default, so what I state is "you either provide a non-empty list or a valid ruleset"

src/elvis_config.erl

elbrujohalcon · 2025-07-31T07:05:02Z

src/elvis_config.erl

+either_rules_is_nonempty_or_ruleset_is_defined(_Rules, Ruleset) when Ruleset =/= undefined ->
+    ok;
+either_rules_is_nonempty_or_ruleset_is_defined(_Rules, _Ruleset) ->
+    {error, io_lib:format("either 'rules' is a non-empty list or 'ruleset' is defined.", [])}.


Maybe something like…

Suggested change

{error, io_lib:format("either 'rules' is a non-empty list or 'ruleset' is defined.", [])}.

{error, io_lib:format("each config section is expected to have either a non-empty list of 'rules' or a 'ruleset'.", [])}.

…would be clearer.

elbrujohalcon · 2025-07-31T07:09:11Z

src/elvis_config.erl

+    ok.
+
+check_flag({Option, _What} = Obj) ->
+    table_exists() andalso ets:lookup(elvis_config, Option) =:= [Obj].


I'm not strongly against your code, but I would've written it as…

Suggested change

table_exists() andalso ets:lookup(elvis_config, Option) =:= [Obj].

try ets:lookup(elvis_config, Option) of

Result -> Result =:= [Obj]

catch

_:badarg -> false

end.

…or even…

Suggested change

table_exists() andalso ets:lookup(elvis_config, Option) =:= [Obj].

try

[Obj] =:= ets:lookup(elvis_config, Option)

catch

_:badarg -> false

end.

elbrujohalcon · 2025-07-31T07:12:31Z

src/elvis_config.erl

+    case table_exists() of
+        false ->
+            _ = ets:new(Table, [public, named_table]);
+        _ ->
+            ok
+    end.


Considering my previous comments, this function may no longer be needed, but if it stays… It has a somewhat semantic issue: Line 791 checks if elvis_config table exists, but line 793 then creates a table called Table. Either both should be parametric or the function should receive no args.

elbrujohalcon · 2025-07-31T07:13:35Z

src/elvis_config.erl

+            NS = elvis_rule:ns(Rule),
+            Name = elvis_rule:name(Rule),
+            % Bypass new/ constraints.
+            DefKeys = maps:keys(NS:default(Name)) ++ [ignore],


Suggested change

DefKeys = maps:keys(NS:default(Name)) ++ [ignore],

DefKeys = [ignore | maps:keys(NS:default(Name))],

elbrujohalcon

I left a bunch of comments here and there… instead of making a formal review, mostly because I was not really conducting a review… just reading the code diagonally :)

In other words: A little bit of nit-picking, I hope you don't mind 🫣

elbrujohalcon

I finally had time to do a full review. All my previous comments stay, and I added yet another nit-picky one. Once all are addressed, this is mergeable to me.
Excellent job, as usual, @paulo-ferraz-oliveira !!

elbrujohalcon · 2025-08-01T12:25:18Z

src/elvis_config.erl

+            ),
+            default_for(Key);
+        {ok, Value} ->
+            elvis_utils:debug("value for key '~s' found in application environment", [Key]),


Suggested change

elvis_utils:debug("value for key '~s' found in application environment", [Key]),

elvis_utils:debug("value for key '~s' (~p) found in application environment", [Key, Value]),

elbrujohalcon · 2025-10-07T06:39:13Z

@paulo-ferraz-oliveira what's the status of this PR? Are you still working on it?

elbrujohalcon · 2025-10-29T08:15:03Z

@paulo-ferraz-oliveira what's the status of this PR? Are you still working on it?

paulo-ferraz-oliveira mentioned this pull request Jul 15, 2025

Improve elvis.config validation #438

Open

paulo-ferraz-oliveira marked this pull request as ready for review July 15, 2025 21:29

paulo-ferraz-oliveira marked this pull request as draft July 15, 2025 21:30

paulo-ferraz-oliveira mentioned this pull request Jul 15, 2025

Validate rule configs in elvis.config #439

Open

elbrujohalcon reviewed Jul 16, 2025

View reviewed changes